feat(audio): live wrapper + RX & TX-mic migration onto the sink factory (#3306)#3398
Conversation
jensenpat
changed the title
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for the careful staging here, @jensenpat — the OS-as-data design carries through cleanly to the wrapper, and isolating it to one live-Qt-Multimedia layer makes the regression matrix a real artifact rather than aspirational. A few notes from a focused read:
Real findings
-
PR title contains an unsubstituted
${PR_TITLE}/${PR_DIFF}placeholder ("live wrapper + RX ${PR_TITLE} TX-mic migration…", and similar in the body). Looks like a templating substitution that didn't run before push. Worth editing the title and the "Full macOS app builds ${PR_DIFF} links clean" line in the body before merge so the squashed commit message lands clean. (Not a code issue, but it propagates to the commit log.) -
m_rxOutputRateis plainint, read from them_rxTimerlambda and written fromstartRxStream()(AudioEngine.cpp:676and:1044). Identical hazard to the priorbool m_resampleTo48k, so not a regression introduced here — but as long as the field is changing shape, it'd be cheap to make itstd::atomic<int>to match the surroundingm_rxPan/m_rxBufferCapMs/m_rxBufferSampleRate(which are allstd::atomic). Drive-by, not blocking. -
Windows Quindar-on-RX is a real, scope-creep-y behavior delta worth flagging on the soak list explicitly — the PR description does mention it, but the body bullet calls it a "strictly-additive win." It's additive in intent, but it does mean Windows users get an additional
QAudioSinkopening alongside RX on a code path that previously short-circuited. Probably fine, but it deserves its own line item in the Windows soak ("verify Quindar local sink opens cleanly, no extra device contention, no Bluetooth-route flip").
Smaller observations
-
startRxStream()skips Int16 ladder rungs (AudioEngine.cpp:1037) because the RX drain is Float PCM. Consistent with the oldmakeFormat()(Float-only), so no regression — but the policy generates Int16 candidates that you then filter out. ADirection::OutputFloatOnlyvariant or apolicy.formatMaskwould letbuildLadder()skip them at source. Pure cleanup, doesn't have to happen now. -
The new RX loop captures
lastRxError = audioErrorName(sink->error())beforedelete sink— good. But on the success path,triedFloatRungis left false on the first rung succeeding, sonoteRxFallback("preferred RX format unavailable -> N Hz")only fires after a real fallback. That reads correctly. -
m_audioSink = nullptr; m_audioDevice = nullptr;at:1030is redundant given the early-return guard at:932, but harmless.
Correctness spot-checks that came back clean
Resampler::dstRate()exists (src/core/Resampler.h:40), so the rebuild check on device-rate change is well-formed.- macOS telephony substitution stays pre-ladder (intentional, matches
#1705). formatLadder()falls back to optimistic first-rung-only on a null/unprobeable device (AudioDeviceNegotiator.cpp:39), which is the right move for WASAPI probe-at-open.- The two stereo resampler strategies (PreservePan vs MonoCollapse) stay distinct in
ResamplerPolicy, with the policy header explicitly calling out the#2403/#2459regression they must not re-introduce.
Architecture is sound and the golden matrix is the right shape for catching this class of bug on a Linux runner. Mostly housekeeping comments above — please fix the title placeholder, and consider the atomic on m_rxOutputRate while you're in there.
🤖 aethersdr-agent · cost: $17.0716 · model: claude-opus-4-7
ten9876
force-pushed
the
aether/audio-sink-factory-live
branch
from
583067c to
0b4a85b
Compare
m_rxOutputRate is written from the GUI thread (startRxStream() at AudioEngine.cpp:1044) and read from the RX drain timer lambda at :676, plus the status JSON path at :851, the scope/status read sites at :1471/3849/5108, and the cross-thread .store() into m_rxBufferSampleRate at :1106. All four surrounding fields (m_rxPan, m_rxBufferCapMs, m_rxBufferSampleRate, m_muted) are already std::atomic<int>/<bool>; the previous bool m_resampleTo48k had the same hazard, so this is not a regression introduced by aethersdr#3398 — but the migration is the cheapest moment to close it. Flagged in the @aethersdr-agent bot review on aethersdr#3398 as a non-blocking drive-by; folding it in here rather than as a follow-up issue keeps the field-shape change and the atomic upgrade in one commit so git blame tells the right story. Mechanical: 16 read sites take .load(), 1 write at :1044 takes .store(), the inline rxOutputResamplingActive() getter in AudioEngine.h gets a .load() too. <atomic> is already pulled in transitively. Verified locally: - audio_format_negotiation_test 25/25 - audio_device_negotiator_test 8/8 - Full AetherSDR build clean (626/626) Principle XI.
|
Rebased against current main (post-#3397 merge) and pushed `0b4a85b3` addressing the one substantive item from @aethersdr-agent's review. What changed in the force-push:
On the other two bot findings:
Verified locally (Arch Linux x86):
Same maintainer-fix-up pattern as #3279/#3286/#3289/#3381/#3391/#3397. Soak on Linux from my side is clean; Windows/macOS soak still wanted before final merge per your description, but CI is green and the code is in a state where soak is the bottleneck, not review. @jensenpat — happy to revert the rebase or the atomic upgrade if you'd rather take a different cut at either. Principle XI. |
Layer 2 of the audio sink factory: the thin, and ONLY, platform-specific
bridge between a real QAudioDevice and the pure AudioFormatNegotiator policy.
- src/core/AudioDeviceNegotiator.{h,cpp}: probe(QAudioDevice) builds a
DeviceCaps snapshot (rate probing via isFormatSupported, preferredFormat,
per-OS isFormatSupportedReliable so Windows WASAPI probes-at-open aethersdr#2120/
aethersdr#2929, caller-fed Bluetooth-HFP hint aethersdr#2615); negotiate() returns a ready-to-
open QAudioFormat + ResamplerKind; formatLadder() returns the ordered Qt
format ladder for try-at-open backends. SampleFmt <-> QAudioFormat mapping
lives here so the policy layer stays Qt-Multimedia-free and headless-testable.
- tests/audio_device_negotiator_test.cpp: smoke test that probes the real
default output/input devices and round-trips to an openable format; tolerant
of headless CI runners with no audio hardware. 8/8 pass on macOS (default
output negotiates 48000/PreservePan, matching the current RX behaviour).
No sink consumes the wrapper yet — the RX/mic migrations land next, per the
incremental plan in docs/audio-sink-factory.md.
Refs aethersdr#3306
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Codex <noreply@openai.com>
…dr#3306) Replace the two forked per-OS #ifdef negotiation blocks in AudioEngine::startRxStream() with a single walk of the factory's Float format ladder (AudioDeviceNegotiator), trying each rung with a real start() so reliable backends and WASAPI probe-at-open behave identically. Net -87 lines. Behaviour is identical for normal devices: Windows/macOS still open 48k (dodging the WASAPI 24k resampler artifacts aethersdr#2120 and keeping macOS A2DP devices off the HFP/telephony route), Linux still opens native 24k. New, strictly-additive wins: - a 44.1 kHz-only output now works (24k->44.1k resample) instead of failing — the aethersdr#3306 / aethersdr#3385 regression the golden matrix guards; - the Quindar local monitor now opens on Windows too (the old Windows branch returned before startQuindarLocalSink()). To support a non-48k device rate, the m_resampleTo48k bool is generalized to an m_rxOutputRate int. resampleStereo() now targets that rate with its two independent L/R r8brain instances (PreservePan — VITA-49 pan preserved, never mono-collapsed here, aethersdr#2403/aethersdr#2459); the RADE decoded-speech path (MonoCollapse) and both TX-scope rate sites follow suit. Cached resamplers rebuild when the device rate changes (e.g. a 48k->44.1k device swap). The macOS telephony/HFP output substitution (aethersdr#1705) is unchanged — it is device selection, applied before the format ladder. Builds clean into the full app; audio_format_negotiation_test (25/25) and audio_device_negotiator_test (8/8, real macOS devices -> 48000/PreservePan) pass. Refs aethersdr#3306 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Codex <noreply@openai.com>
…ethersdr#3306) macOS + Linux: AudioEngine::startTxStream() now drives the mic rate/format selection from the factory's Int16 Input ladder (AudioDeviceNegotiator), walking stereo-then-mono to preserve the existing channel fallback. This removes the per-OS #ifdef rate lists and the bespoke macTxInputRateCandidates() helper — its policy (preferred-rate-first to dodge the silent-48k-open trap aethersdr#2930; Bluetooth-HFP native rate first aethersdr#2615) now lives in the one factory ladder. The macOS CoreAudio-HAL detection the factory can't derive (macBluetoothNativeInputRate) is fed in via a new preferredRateOverride on the wrapper, so a BT-HFP mic still opens at its native low rate. Behaviour preserved: standard macOS mic -> preferred 48k (then 44.1k/24k/16k); Linux -> native 24k (then 48k/44.1k); BT-HFP mic -> native low rate first. The existing TX resampler (m_txInputRate -> 24k) and the push/pull open modes, watchdog, and metering are unchanged. Windows mic is intentionally left as-is: it already matches the factory's Windows policy (force 48k + WASAPI probe-at-open) and carries the mono-only USB-mic channel clamp (aethersdr#2929) that warrants its own soak — the remaining mic increment. Builds & links clean into the full app; both negotiation tests still pass. Refs aethersdr#3306 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Codex <noreply@openai.com>
m_rxOutputRate is written from the GUI thread (startRxStream() at AudioEngine.cpp:1044) and read from the RX drain timer lambda at :676, plus the status JSON path at :851, the scope/status read sites at :1471/3849/5108, and the cross-thread .store() into m_rxBufferSampleRate at :1106. All four surrounding fields (m_rxPan, m_rxBufferCapMs, m_rxBufferSampleRate, m_muted) are already std::atomic<int>/<bool>; the previous bool m_resampleTo48k had the same hazard, so this is not a regression introduced by aethersdr#3398 — but the migration is the cheapest moment to close it. Flagged in the @aethersdr-agent bot review on aethersdr#3398 as a non-blocking drive-by; folding it in here rather than as a follow-up issue keeps the field-shape change and the atomic upgrade in one commit so git blame tells the right story. Mechanical: 16 read sites take .load(), 1 write at :1044 takes .store(), the inline rxOutputResamplingActive() getter in AudioEngine.h gets a .load() too. <atomic> is already pulled in transitively. Verified locally: - audio_format_negotiation_test 25/25 - audio_device_negotiator_test 8/8 - Full AetherSDR build clean (626/626) Principle XI.
… log Final-pass cleanup: the RX-speaker migration entry in the design doc said "generalized to an m_rxOutputRate int", but the atomic upgrade earlier in this PR (0b4a85b) makes the final shape std::atomic<int>. Future readers of the migration log shouldn't have to chase the header to find out the field is atomic. Principle XI.
ten9876
force-pushed
the
aether/audio-sink-factory-live
branch
from
0b4a85b to
908f8a1
Compare
Two related fixes to TitleBar's new PanLock accessors so all four platforms compile again: 1. signals: vs. public: — MOC parses anything inside a `signals:` block as a signal declaration, so the inline `bool isPanFollowChecked() const` and `void setPanFollowChecked(bool)` were rejected at moc time with "Not a signal declaration" (TitleBar.h:56). Both methods are accessors, not signals — they belong in `public:`. 2. Out-of-line over inline. Even after the section move, the inline bodies referenced `m_panFollowBtn->isChecked()` and `QSignalBlocker`, which need the full QPushButton type. The header only forward-declares QPushButton, so inline bodies couldn't compile in callers either (the moc error masked this until now). Moving the definitions to TitleBar.cpp keeps the header light and matches the style of neighbours like `isSystemMoveAreaAt`. Build verified locally on Arch Linux x86 — 632/632 clean (only the pre-existing unrelated macDaxDriverInstalled warning). Same maintainer fix-up pattern as aethersdr#3279/aethersdr#3286/aethersdr#3289/aethersdr#3381/aethersdr#3398/aethersdr#3417/aethersdr#3439/aethersdr#3441. Principle XI.
Two related fixes to TitleBar's new PanLock accessors so all four platforms compile again: 1. signals: vs. public: — MOC parses anything inside a `signals:` block as a signal declaration, so the inline `bool isPanFollowChecked() const` and `void setPanFollowChecked(bool)` were rejected at moc time with "Not a signal declaration" (TitleBar.h:56). Both methods are accessors, not signals — they belong in `public:`. 2. Out-of-line over inline. Even after the section move, the inline bodies referenced `m_panFollowBtn->isChecked()` and `QSignalBlocker`, which need the full QPushButton type. The header only forward-declares QPushButton, so inline bodies couldn't compile in callers either (the moc error masked this until now). Moving the definitions to TitleBar.cpp keeps the header light and matches the style of neighbours like `isSystemMoveAreaAt`. Build verified locally on Arch Linux x86 — 632/632 clean (only the pre-existing unrelated macDaxDriverInstalled warning). Same maintainer fix-up pattern as aethersdr#3279/aethersdr#3286/aethersdr#3289/aethersdr#3381/aethersdr#3398/aethersdr#3417/aethersdr#3439/aethersdr#3441. Principle XI.
2 participants
What
Layer 2 + the first two sink migrations of the consolidated audio sink factory (#3306):
AudioDeviceNegotiator— the thin, and only, platform-specific bridge.probe(QAudioDevice)→ pureDeviceCaps;negotiate()/formatLadder()→ openableQAudioFormats. Smoke-tested against the real default devices (audio_device_negotiator_test, 8/8).startRxStream()walks the factory's Float ladder with realstart()attempts instead of two forked per-OS#ifdefblocks (net −87 lines).m_resampleTo48k(bool) →m_rxOutputRate(int).startTxStream()drives mic rate/format from the factory's Int16 Input ladder (stereo-then-mono), removing the per-OS rate lists and the bespokemacTxInputRateCandidates()helper.Behaviour (preserved)
returned beforestartQuindarLocalSink()).macBluetoothNativeInputRate()+ the wrapper's newpreferredRateOverride; preferred-rate-first avoids the silent-48k-open trap (fix(audio): silent SSB/voice TX on macOS — honour mic native rate + start capture for remote_audio_tx #2930).Scope notes / invariants
AudioOutputRouter, TCI-TX client-rate, and PipeWire DAX are the remaining increments (seedocs/audio-sink-factory.md).Test / build
audio_format_negotiation_test25/25,audio_device_negotiator_test8/8.Refs #3306
💻 Generated with Claude Code (Opus 4.8) with architecture by @jensenpat